fix(server): align ProtocolError re-throw with spec error classification#1769
fix(server): align ProtocolError re-throw with spec error classification#1769felixweinberger wants to merge 2 commits intomainfrom
Conversation
Re-throw all ProtocolError instances from the tools/call handler as JSON-RPC errors. Previously only UrlElicitationRequired was re-thrown; other ProtocolErrors thrown inside the try block (output validation, task misconfiguration) were wrapped as isError: true tool results. Per the MCP spec's error classification: - Input validation failures are tool-execution errors (isError: true) - Output validation failures are server errors (JSON-RPC InternalError) - Task misconfiguration is a protocol mismatch (JSON-RPC error) Changes: - validateToolInput now throws plain Error so input validation stays tool-level (isError: true) - validateToolOutput now uses InternalError code instead of InvalidParams (output validation failure is a server-side bug, not client fault) - catch block re-throws any ProtocolError, matching python-sdk semantics and allowing tool handlers to deliberately throw protocol-level errors
🦋 Changeset detectedLatest commit: e6b8a57 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
There was a problem hiding this comment.
The code change itself is small and logically sound, but this is an explicitly breaking change to core error handling in the tools/call handler that warrants human sign-off — particularly the design choice to keep MethodNotFound for task-required-without-task (vs InvalidParams), and whether the changeset should be minor given the breaking nature.
Extended reasoning...
Overview
This PR broadens the catch block in McpServer's tools/call handler from only re-throwing UrlElicitationRequired to re-throwing all ProtocolError instances. It also downgrades validateToolInput from throwing ProtocolError to plain Error (so input validation stays as isError: true), and changes output validation error codes from InvalidParams to InternalError. Documentation and tests are updated accordingly.
Security risks
No security concerns. The change affects error classification, not authorization or data exposure.
Level of scrutiny
This is a self-described breaking change to a core code path. The tools/call handler is exercised by every tool invocation. Consumers relying on result.isError to detect output validation failures or handler-thrown ProtocolErrors will see different behavior. The changeset marks it as minor but the PR description and migration docs both call it breaking — a human should verify the semver classification. The files are also covered by CODEOWNERS (@modelcontextprotocol/typescript-sdk).
Other factors
The two bug reports are both pre-existing issues not introduced by this PR. The code logic is correct and well-motivated (aligning with spec classification and Python SDK behavior). Tests are updated to match. Migration docs are thorough. The concern is purely about the design-level decisions that a maintainer should validate.
| // Return the final result | ||
| return (await ctx.task.store.getTaskResult(taskId)) as CallToolResult; | ||
| const result = (await ctx.task.store.getTaskResult(taskId)) as CallToolResult; | ||
| await this.validateToolOutput(tool, result, request.params.name); |
There was a problem hiding this comment.
🟡 validateToolOutput at line 336 runs unconditionally after the polling loop, but the loop (line 325) also exits on failed and cancelled status. When a failed task stores a result without isError: true and without structuredContent, and the tool has an outputSchema, validateToolOutput throws a misleading ProtocolError(InternalError, "has output schema but no structured content") — masking the actual task failure. Guard with if (task.status \!== "completed") before lines 335-336.
Extended reasoning...
What the bug is
After the polling loop at line 325, handleAutomaticTaskPolling unconditionally calls getTaskResult and validateToolOutput at lines 335-336. However, the while loop exits when task.status is any of completed, failed, or cancelled. For failed or cancelled tasks, calling validateToolOutput is semantically wrong — a failed task should surface its failure reason, not have its output validated against a schema.
The triggering code path
- A tool is registered with
taskSupport: "optional", anoutputSchema, and a task handler. - A client calls this tool WITHOUT task augmentation (
request.params.taskis falsy). - Execution enters
handleAutomaticTaskPollingat line 193. - The task handler runs, but the task eventually fails. The handler calls
storeTaskResultwith a result that hascontentbut does NOT setisError: trueand does NOT includestructuredContent. This is valid — the task status field (failed) indicates failure, not theisErrorflag. - The polling loop exits because
task.status === "failed". - At line 335,
getTaskResultretrieves the stored result successfully. - At line 336,
validateToolOutputis called. It checks:outputSchemaexists (yes),content in result(yes),result.isError(false — not set),result.structuredContent(undefined). It then throwsProtocolError(InternalError, "has an output schema but no structured content was provided").
Why existing guards do not prevent this
validateToolOutput has three early-return guards: (1) no outputSchema, (2) result is not a CallToolResult, (3) result.isError is true. For the described scenario, the tool has an outputSchema, the result is a CallToolResult with content, and isError is not set (since storeTaskResult does not enforce isError: true for failed tasks — confirmed by the test at inMemory.test.ts:535). None of the guards trigger.
Impact
The misleading ProtocolError about output schema masks the actual task failure. Instead of the client learning that the task failed (and why), it receives an opaque internal error about missing structured content. This is a narrow edge case requiring: taskSupport: "optional" + no task augmentation from client + task fails + result stored without isError: true + tool has outputSchema. But it was introduced by this PR (the validateToolOutput call at line 336 is new).
How to fix
Guard the getTaskResult/validateToolOutput calls with a status check:
if (task.status \!== "completed") {
const result = await ctx.task.store.getTaskResult(taskId).catch(() => null);
const message = result && "content" in result
? (result.content as Array<{text?: string}>)?.[0]?.text ?? `Task ${task.status}`
: `Task ${task.status}`;
throw new Error(message);
}
const result = (await ctx.task.store.getTaskResult(taskId)) as CallToolResult;
await this.validateToolOutput(tool, result, request.params.name);
return result;Or more minimally, just guard validateToolOutput:
const result = (await ctx.task.store.getTaskResult(taskId)) as CallToolResult;
if (task.status === "completed") {
await this.validateToolOutput(tool, result, request.params.name);
}
return result;
Re-throws all
ProtocolErrorinstances from thetools/callhandler as JSON-RPC errors. Previously onlyUrlElicitationRequiredwas re-thrown; otherProtocolErrors thrown inside the try block (output validation, task misconfiguration) were silently wrapped asisError: truetool results.Motivation and Context
The MCP spec classifies tool errors into two categories:
isError: true): API failures, input validation errors, business logic errorsThe current catch block only re-throws
UrlElicitationRequired, which means output validation failures (a server-side bug) and task misconfiguration errors get demoted to tool-levelisError: trueresults when they should be protocol-level JSON-RPC errors.This also means tool handlers that deliberately
throw new ProtocolError(...)get their intent overridden — the python-sdk re-throws allMCPErrorin the equivalent path.How Has This Been Tested?
Updated existing integration tests to reflect the new behavior. All tests pass locally.
Breaking Changes
Yes — output validation failures and task-required-without-task now throw
ProtocolErrorinstead of returning{ isError: true }. See migration guide updates.Input validation behavior is unchanged (still
isError: true, per spec).Types of changes
Checklist
Additional context
Surfaced while triaging #1674, which proposed the broadened re-throw but would have also promoted input validation to JSON-RPC (spec violation). This PR applies the broadening while keeping input validation tool-level by changing
validateToolInputto throw plainErrorinstead ofProtocolError.